Feat/add legacy sig message#135
Conversation
WalkthroughThe changes update various modules to include an additional Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Signer as Chain Module
Client->>Signer: sign_message(private_key, message, legacy)
alt legacy is true
Signer->>Signer: prepare_message_legacy(message)
Signer->>Signer: Execute legacy signing process
else legacy is false
Signer->>Signer: prepare_message(message)
Signer->>Signer: Execute standard signing process
end
Signer-->>Client: Return signature
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kos/src/chains/klv/mod.rs (1)
122-125: Misspelled variable nameThere's a typo in the variable name "prepared_messafe" which should be "prepared_message".
- let prepared_messafe = KLV::prepare_message(message); + let prepared_message = KLV::prepare_message(message);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/kos-codec/src/chains/ada/mod.rs(1 hunks)packages/kos-hardware/src/lib.rs(2 hunks)packages/kos-mobile/src/lib.rs(2 hunks)packages/kos-web/src/wallet.rs(3 hunks)packages/kos/src/chains/ada/mod.rs(5 hunks)packages/kos/src/chains/apt/mod.rs(2 hunks)packages/kos/src/chains/atom/mod.rs(3 hunks)packages/kos/src/chains/bch/mod.rs(2 hunks)packages/kos/src/chains/btc/mod.rs(3 hunks)packages/kos/src/chains/eth/mod.rs(2 hunks)packages/kos/src/chains/icp/mod.rs(2 hunks)packages/kos/src/chains/klv/mod.rs(2 hunks)packages/kos/src/chains/mod.rs(2 hunks)packages/kos/src/chains/sol/mod.rs(2 hunks)packages/kos/src/chains/substrate/mod.rs(2 hunks)packages/kos/src/chains/sui/mod.rs(3 hunks)packages/kos/src/chains/trx/mod.rs(2 hunks)packages/kos/src/chains/xrp/mod.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/kos/src/chains/atom/mod.rs (1)
Learnt from: gustavocbritto
PR: klever-io/kos-rs#122
File: packages/kos/src/chains/atom/mod.rs:124-0
Timestamp: 2025-04-10T18:52:10.679Z
Learning: The ATOM chain implementation in kos-rs requires a [27] prefix byte for validating compressed signatures, which differs from the standard Cosmos 64-byte signature format. This is an intentional implementation detail.
🧬 Code Graph Analysis (13)
packages/kos/src/chains/btc/mod.rs (6)
packages/kos/src/chains/xrp/mod.rs (3)
prepare_message_legacy(35-38)sign_message(147-159)new(20-22)packages/kos/src/crypto/hash.rs (1)
sha256_digest(5-12)packages/kos/src/chains/ada/mod.rs (2)
sign_message(133-150)new(18-20)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/mod.rs (2)
sign_message(389-394)new(411-761)
packages/kos/src/chains/apt/mod.rs (9)
packages/kos/src/chains/ada/mod.rs (1)
sign_message(133-150)packages/kos/src/chains/icp/mod.rs (1)
sign_message(124-139)packages/kos/src/chains/eth/mod.rs (1)
sign_message(152-186)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/trx/mod.rs (1)
sign_message(149-165)packages/kos/src/chains/substrate/mod.rs (1)
sign_message(187-194)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/xrp/mod.rs (1)
sign_message(147-159)packages/kos/src/chains/mod.rs (1)
sign_message(389-394)
packages/kos-codec/src/chains/ada/mod.rs (2)
packages/kos/src/chains/ada/mod.rs (1)
new(18-20)packages/kos/src/chains/mod.rs (1)
new(411-761)
packages/kos/src/chains/klv/mod.rs (12)
packages/kos/src/chains/apt/mod.rs (2)
sign_message(76-93)test_sign_message(135-151)packages/kos/src/chains/ada/mod.rs (2)
sign_message(133-150)test_sign_message(194-209)packages/kos/src/chains/atom/mod.rs (2)
sign_message(130-146)test_sign_message(187-204)packages/kos/src/chains/icp/mod.rs (1)
sign_message(124-139)packages/kos/src/chains/eth/mod.rs (2)
sign_message(152-186)test_sign_message(345-362)packages/kos/src/chains/sol/mod.rs (2)
sign_message(94-101)test_sign_message(269-280)packages/kos/src/chains/trx/mod.rs (2)
sign_message(149-165)test_sign_message(353-370)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/sui/mod.rs (2)
sign_message(76-102)test_sign_message(143-160)packages/kos/src/chains/xrp/mod.rs (1)
sign_message(147-159)packages/kos/src/chains/btc/mod.rs (2)
sign_message(287-309)test_sign_message(434-445)packages/kos/src/chains/mod.rs (1)
sign_message(389-394)
packages/kos/src/chains/sol/mod.rs (14)
packages/kos/src/chains/apt/mod.rs (1)
sign_message(76-93)packages/kos/src/chains/ada/mod.rs (1)
sign_message(133-150)packages/kos/src/chains/atom/mod.rs (1)
sign_message(130-146)packages/kos/src/chains/icp/mod.rs (1)
sign_message(124-139)packages/kos/src/chains/eth/mod.rs (1)
sign_message(152-186)packages/kos/src/chains/klv/mod.rs (1)
sign_message(116-125)packages/kos/src/chains/trx/mod.rs (1)
sign_message(149-165)packages/kos/src/chains/substrate/mod.rs (1)
sign_message(187-194)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/sui/mod.rs (1)
sign_message(76-102)packages/kos/src/chains/xrp/mod.rs (1)
sign_message(147-159)packages/kos/src/chains/btc/mod.rs (1)
sign_message(287-309)packages/kos/src/chains/mod.rs (1)
sign_message(389-394)packages/kos/src/chains/sol/models.rs (2)
encode(172-222)encode(248-264)
packages/kos/src/chains/sui/mod.rs (14)
packages/kos/src/chains/apt/mod.rs (1)
sign_message(76-93)packages/kos/src/chains/ada/mod.rs (1)
sign_message(133-150)packages/kos/src/chains/atom/mod.rs (1)
sign_message(130-146)packages/kos/src/chains/icp/mod.rs (1)
sign_message(124-139)packages/kos/src/chains/eth/mod.rs (1)
sign_message(152-186)packages/kos/src/chains/klv/mod.rs (1)
sign_message(116-125)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/trx/mod.rs (1)
sign_message(149-165)packages/kos/src/chains/substrate/mod.rs (1)
sign_message(187-194)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos-mobile/src/lib.rs (1)
sign_message(319-325)packages/kos/src/chains/xrp/mod.rs (1)
sign_message(147-159)packages/kos/src/chains/btc/mod.rs (1)
sign_message(287-309)packages/kos/src/chains/mod.rs (1)
sign_message(389-394)
packages/kos/src/chains/ada/mod.rs (9)
packages/kos/src/chains/eth/mod.rs (2)
new(33-35)sign_message(152-186)packages/kos/src/chains/xrp/mod.rs (2)
new(20-22)sign_message(147-159)packages/kos/src/chains/mod.rs (2)
new(411-761)sign_message(389-394)packages/kos/src/chains/apt/mod.rs (1)
sign_message(76-93)packages/kos/src/chains/icp/mod.rs (1)
sign_message(124-139)packages/kos/src/chains/klv/mod.rs (1)
sign_message(116-125)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/trx/mod.rs (1)
sign_message(149-165)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)
packages/kos/src/chains/eth/mod.rs (4)
packages/kos/src/chains/icp/mod.rs (1)
sign_message(124-139)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/xrp/mod.rs (2)
sign_message(147-159)new(20-22)packages/kos/src/chains/mod.rs (2)
sign_message(389-394)new(411-761)
packages/kos/src/chains/bch/mod.rs (10)
packages/kos/src/chains/apt/mod.rs (1)
sign_message(76-93)packages/kos/src/chains/ada/mod.rs (2)
sign_message(133-150)new(18-20)packages/kos/src/chains/icp/mod.rs (1)
sign_message(124-139)packages/kos/src/chains/eth/mod.rs (2)
sign_message(152-186)new(33-35)packages/kos/src/chains/klv/mod.rs (1)
sign_message(116-125)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/trx/mod.rs (1)
sign_message(149-165)packages/kos/src/chains/xrp/mod.rs (2)
sign_message(147-159)new(20-22)packages/kos/src/chains/btc/mod.rs (2)
sign_message(287-309)new(33-35)packages/kos/src/chains/mod.rs (2)
sign_message(389-394)new(411-761)
packages/kos-web/src/wallet.rs (7)
packages/kos/src/chains/apt/mod.rs (1)
sign_message(76-93)packages/kos/src/chains/eth/mod.rs (1)
sign_message(152-186)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/substrate/mod.rs (1)
sign_message(187-194)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/xrp/mod.rs (1)
sign_message(147-159)packages/kos/src/chains/btc/mod.rs (1)
sign_message(287-309)
packages/kos-mobile/src/lib.rs (8)
packages/kos/src/chains/eth/mod.rs (1)
sign_message(152-186)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/substrate/mod.rs (1)
sign_message(187-194)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/sui/mod.rs (1)
sign_message(76-102)packages/kos/src/chains/xrp/mod.rs (1)
sign_message(147-159)packages/kos/src/chains/btc/mod.rs (1)
sign_message(287-309)packages/kos/src/chains/mod.rs (1)
sign_message(389-394)
packages/kos/src/chains/xrp/mod.rs (9)
packages/kos/src/chains/btc/mod.rs (3)
prepare_message_legacy(118-120)new(33-35)sign_message(287-309)packages/kos/src/crypto/hash.rs (1)
sha256_digest(5-12)packages/kos/src/chains/util.rs (1)
private_key_from_vec(14-19)packages/kos/src/chains/atom/mod.rs (2)
new(34-42)sign_message(130-146)packages/kos/src/chains/apt/mod.rs (1)
sign_message(76-93)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/trx/mod.rs (1)
sign_message(149-165)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/sui/mod.rs (1)
sign_message(76-102)
packages/kos/src/chains/mod.rs (11)
packages/kos/src/chains/apt/mod.rs (1)
sign_message(76-93)packages/kos/src/chains/ada/mod.rs (2)
sign_message(133-150)new(18-20)packages/kos/src/chains/atom/mod.rs (2)
sign_message(130-146)new(34-42)packages/kos/src/chains/icp/mod.rs (1)
sign_message(124-139)packages/kos/src/chains/eth/mod.rs (2)
sign_message(152-186)new(33-35)packages/kos/src/chains/sol/mod.rs (1)
sign_message(94-101)packages/kos/src/chains/trx/mod.rs (1)
sign_message(149-165)packages/kos/src/chains/bch/mod.rs (1)
sign_message(175-183)packages/kos/src/chains/sui/mod.rs (1)
sign_message(76-102)packages/kos/src/chains/xrp/mod.rs (2)
sign_message(147-159)new(20-22)packages/kos/src/chains/btc/mod.rs (2)
sign_message(287-309)new(33-35)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (52)
packages/kos-codec/src/chains/ada/mod.rs (1)
115-115: Clean ADA initialization updateThe code correctly updates the instantiation of ADA to use the new constructor signature without the boolean parameter. This change aligns with the simplification of the ADA struct in the core package.
packages/kos-hardware/src/lib.rs (2)
180-180: Added support for legacy signing parameterThe function signature now includes a
legacy: boolparameter, allowing callers to specify whether to use legacy signing methods.
195-195: Updated function call to pass legacy parameterThe code correctly passes the
legacyparameter to the chain'ssign_messagemethod, ensuring that the legacy signing preference is respected.packages/kos/src/chains/sol/mod.rs (2)
94-99: Updated sign_message signature to include legacy parameterThe method signature has been properly updated to include the
_legacyparameter, maintaining compatibility with the Chain trait. The underscore prefix indicates that this parameter is intentionally unused in the Solana implementation.
275-277: Updated test case with legacy parameterThe test case has been correctly updated to pass
falsefor the legacy parameter, ensuring compatibility with the new method signature.packages/kos/src/chains/btc/mod.rs (2)
118-120: Added legacy message preparation methodThe new
prepare_message_legacymethod provides support for legacy Bitcoin message signing by performing a double SHA-256 hash of the input message, which differs from the standard message preparation that includes the Bitcoin message prefix.
287-309: Enhanced sign_message to support legacy signingThe implementation now properly handles both legacy and non-legacy signing modes:
- When
legacyis true, it usesprepare_message_legacyand includes a recovery byte in the signature- When
legacyis false, it continues to use the standard message preparationThis implementation aligns with other chain implementations like XRP and SUI that also handle the legacy parameter.
packages/kos/src/chains/apt/mod.rs (2)
76-81: Implementation supports new legacy parameter.The
sign_messagemethod signature has been correctly updated to include the new_legacy: boolparameter in accordance with the Chain trait changes. The underscore prefix indicates it's currently unused in this implementation.
146-150: Test updated for new signature with appropriate test values.The test has been properly updated to:
- Use a consistent "test message" value
- Pass the new required
legacyparameter (set totrue)- Update the expected signature hash to match the new test conditions
This ensures proper verification of the signing functionality with the updated interface.
packages/kos/src/chains/mod.rs (2)
389-394: Chain trait signature updated to include legacy parameter.The
sign_messagemethod in theChaintrait has been properly updated to include thelegacy: boolparameter. This is a breaking change that requires updates in all chain implementations, which appears to be handled in this PR.
661-661: ADA instantiation simplified.The
ADA::new()call has been simplified by removing thefalseargument, which aligns with the changes in the ADA implementation where the constructor no longer requires this parameter.packages/kos/src/chains/icp/mod.rs (3)
124-129: Implementation supports new legacy parameter.The
sign_messagemethod signature has been properly updated to include the new_legacy: boolparameter to comply with the Chain trait modification. The underscore prefix indicates it's currently unused in this implementation.
257-259: Test updated to work with new signature.The test has been correctly updated to use a consistent "test message" value and pass the new required
legacyparameter (set totrue).
262-265: Added explicit signature verification.A new assertion has been added to verify the exact expected signature. This improves the test by ensuring that the signature generation process remains consistent across versions.
packages/kos/src/chains/eth/mod.rs (3)
152-157: Implementation supports new legacy parameter.The
sign_messagemethod signature has been properly updated to include the new_legacy: boolparameter to comply with the Chain trait changes. The parameter is currently unused (prefixed with underscore), but maintains compatibility with the updated interface.
340-340: Updated test call with new parameter.The test call to
sign_messagehas been properly updated to passfalseas the third argument to match the new signature.
344-362: Added comprehensive test case for message signing.A new dedicated test function has been added to specifically test the standard message signing functionality. This improves the test coverage by:
- Using a standard test message consistent with other chain implementations
- Explicitly setting the legacy parameter to
true- Verifying the expected signature in hex format
This ensures the message signing functionality behaves as expected with the updated interface.
packages/kos/src/chains/sui/mod.rs (4)
70-72: Update to sign_tx implementationThe sign_tx method now passes
falseto the sign_message method for the newlegacyparameter, indicating that the transaction signature should use the non-legacy format.
76-81: Added legacy parameter to sign_message method signatureThe sign_message method signature has been updated to include a new boolean parameter
legacyto align with the Chain trait's updated interface. This allows choosing between legacy and new signature formats when signing messages.
89-92: Conditional prefix byte based on legacy flagThe implementation now conditionally adds a 0x00 byte to the signature response only when not in legacy mode. This maintains backward compatibility with legacy consumers while allowing new signature format.
142-160: Added test for sign_message with legacy flagA comprehensive test has been added to verify the correct functionality of the sign_message method with the new legacy parameter set to false. The test validates that the generated signature matches the expected format.
packages/kos/src/chains/klv/mod.rs (2)
116-121: Added legacy parameter to sign_message method signatureThe sign_message method signature has been updated to include the new
_legacyboolean parameter to match the Chain trait's updated interface. The parameter is prefixed with an underscore indicating it's not used in the implementation, which is appropriate since KLV doesn't have different signing behavior for legacy mode.
427-445: Added test for sign_message with legacy flagA test case has been added to verify the sign_message method produces the correct signature when called with the legacy parameter set to false. This ensures the method's behavior remains correct after the signature change.
packages/kos/src/chains/bch/mod.rs (2)
175-183: Updated sign_message method to support legacy parameterThe sign_message method has been updated to include the legacy parameter and pass it through to the BTC implementation. This maintains consistent behavior with the BTC chain when signing messages in both legacy and non-legacy modes.
264-268: Updated test to use legacy modeThe test for sign_message has been updated to explicitly use legacy mode (true) and the expected signature has been updated to match the new behavior. This ensures that the BCH chain correctly handles legacy signatures by delegating to BTC.
packages/kos/src/chains/atom/mod.rs (5)
122-125: Improved sign_tx implementationThe sign_tx method has been refactored to store the sign_raw result in a separate variable before slicing, improving code readability and maintainability.
130-135: Added legacy parameter to sign_message method signatureThe sign_message method signature has been updated to include the new
_legacyboolean parameter to align with the Chain trait's updated interface. The underscore prefix indicates it's not currently used in the implementation.
140-144: Enhanced signature format constructionThe sign_message implementation now properly extracts the recovery byte from the signature and adds it to 27 before appending it to the result. This change aligns with the established ATOM signature format which requires a [27] prefix byte for validating compressed signatures.
154-154: Return full signature from sign_rawThe sign_raw method now returns the complete 65-byte signature rather than slicing it, allowing callers to determine which parts of the signature they need. This provides more flexibility for different signing requirements.
198-203: Updated test to use non-legacy modeThe test for sign_message has been updated to explicitly pass the legacy parameter as false, and the expected signature has been updated to match the new behavior. This ensures that the signature format is correctly generated.
packages/kos/src/chains/substrate/mod.rs (2)
187-194: Function signature updated to support legacy parameter.The
sign_messagefunction has been updated to include an additional_legacy: boolparameter, aligning with the changes to theChaintrait. The underscore prefix indicates that the parameter is currently unused in this implementation.
716-729: Test added for the updatedsign_messagefunction.This test properly validates the
sign_messagefunction with the new signature, ensuring that it produces the expected output. The test is comprehensive and covers the key steps: creating a Substrate instance, generating keys, signing a message, and verifying the result against a known value.packages/kos/src/chains/trx/mod.rs (2)
149-154: Function signature updated to support legacy parameter.The
sign_messagefunction has been updated to include an additional_legacy: boolparameter, aligning with the changes to theChaintrait. The underscore prefix indicates that the parameter is currently unused in this implementation.
352-370: Test added for the updatedsign_messagefunction.A comprehensive test for the updated
sign_messagefunction has been added, validating that it correctly signs a test message and produces the expected signature. The test follows the pattern used across the codebase: initialize the chain, derive keys, sign a message, and verify the result.packages/kos-mobile/src/lib.rs (3)
319-319: Updated function signature to include the legacy parameter.The
sign_messagefunction in the mobile library has been updated to include thelegacy: boolparameter, which will be passed to the chain implementation.
322-323: Updated chain.sign_message call to pass the legacy parameter.The call to
chain.sign_messagehas been updated to include the newlegacyparameter, ensuring it's properly passed from the mobile library to the underlying chain implementation.
639-639: Updated test to use the new function signature with legacy parameter.The test for
sign_messagenow includes thelegacyparameter set totrue, properly testing the updated function signature.packages/kos/src/chains/ada/mod.rs (7)
15-15: Simplified ADA struct by removing the extended_key field.The
ADAstruct has been simplified to an empty struct by removing theextended_keyboolean field. This simplification is in line with the project's objective to streamline the code.
18-20: Updated constructor to remove the extended_key parameter.The
newconstructor has been updated to no longer accept theextended_keyparameter, reflecting the removal of this field from the struct.
23-26: Added Default implementation for ADA.A
Defaultimplementation has been added for theADAstruct, which calls thenewconstructor. This is a good practice that allows for more idiomatic instantiation of the struct.
84-84: Simplified code by removing extended_key logic.The conditional logic that previously checked the
extended_keyfield has been removed, resulting in a direct return of thevkvariable. This simplifies the code and improves readability.
133-138: Updated function signature to support legacy parameter.The
sign_messagefunction has been updated to include an additional_legacy: boolparameter, aligning with the changes to theChaintrait. The underscore prefix indicates that the parameter is currently unused in this implementation.
178-178: Updated test instantiation to use the new constructor signature.The test code has been updated to instantiate the
ADAstruct without passing any parameters, reflecting the simplified constructor.Also applies to: 198-198
204-208: Updated test to use the new sign_message signature with legacy parameter.The test for
sign_messagehas been updated to include thelegacyparameter set tofalse, properly testing the updated function signature.packages/kos-web/src/wallet.rs (4)
328-328: Addedlegacyparameter to sign_message function signature.The method now accepts a boolean
legacyparameter to support different message signing formats, aligning with changes made to the underlying chain implementations. This provides flexibility for clients that need to use legacy signing methods.
336-336: Correctly propagates the legacy parameter to the chain implementation.The updated call passes the
legacyparameter to the chain'ssign_messagemethod, ensuring that the appropriate signing method is used based on the parameter value.
493-493: Updated test call to include the new legacy parameter.The test has been properly updated to include the
legacyparameter set totrue, which tests the legacy signing functionality.
559-559: Updated read-only wallet test to include the legacy parameter.This test case correctly verifies that signing operations fail for read-only wallets, with the updated signature that includes the
legacyparameter.packages/kos/src/chains/xrp/mod.rs (4)
35-38: Added new function for legacy message preparation.This function implements the legacy approach for message preparation using double SHA-256 hashing, which differs from the current implementation that uses SHA-512. This allows for compatibility with systems expecting the older hashing method.
53-72: Implemented new legacy message signing function.This function correctly handles the legacy signing process by:
- Preparing the message using the double SHA-256 approach
- Converting the private key to the appropriate format
- Signing the message using Secp256K1
- Constructing the signature format with the recovery byte and R/S components
The implementation follows a similar pattern to other signing functions in the codebase.
147-159: Updated sign_message function to support both legacy and current methods.The function now checks the
legacyparameter and routes to the appropriate implementation:
- If
legacyis true, it uses the newsign_message_legacyfunction- Otherwise, it continues to use the current implementation with SHA-512 hashing
This maintains backward compatibility while supporting both signing methods.
209-217: Added tests for both legacy and current message signing methods.The test case has been expanded to verify:
- The non-legacy signing method (lines 209-213)
- The new legacy signing method (lines 215-217)
Each test case verifies the expected signature output, ensuring both methods work correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kos/src/chains/ada/mod.rs (1)
140-145: The_legacyparameter is added but not used.The new parameter aligns with the PR objective to add a legacy flag to message signing functions, but it's currently unused (as indicated by the underscore prefix). Consider adding documentation explaining its intended purpose for future developers.
fn sign_message( &self, private_key: Vec<u8>, message: Vec<u8>, + /// Boolean flag to indicate whether to use legacy signing method _legacy: bool, ) -> Result<Vec<u8>, ChainError> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
packages/kos-codec/src/chains/ada/mod.rs(2 hunks)packages/kos/src/chains/ada/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kos-codec/src/chains/ada/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (5)
packages/kos/src/chains/ada/mod.rs (5)
15-15: Simplified ADA struct definition looks good.Removing the
extended_keyfield reduces complexity and state management requirements. This streamlines the implementation.
18-20: Updated constructor correctly initializes the simplified struct.The constructor now properly returns an instance of the empty
ADAstruct, which aligns with the struct's new definition.
23-26: Good addition of Default implementation.Adding a Default implementation for the ADA struct improves usability, allowing for easier instantiation in contexts requiring default values.
185-185: Test initialization updated correctly.The test has been properly updated to use the new constructor signature.
213-213: Test properly includes new parameter.The test for sign_message has been updated to include the
_legacyparameter, maintaining test coverage with the interface changes.
…s-rs into feat/add-legacy-sig-message
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/kos/src/chains/sui/mod.rs (3)
76-81: Method signature updated to include legacy parameter.The signature update aligns with the broader changes across chain implementations. Consider adding documentation explaining what the
legacyparameter signifies and when to use each mode.fn sign_message( &self, private_key: Vec<u8>, message: Vec<u8>, + /// Whether to use legacy signing format (without the leading 0x00 byte) legacy: bool, ) -> Result<Vec<u8>, ChainError> {
90-92: Conditional logic handles legacy signing mode.The implementation now conditionally includes the
0x00byte in the response based on thelegacyflag, which aligns with the expected behavior. However, consider adding a comment to explain the significance of this byte in the signature format.if !legacy { + // Add 0x00 byte as signature scheme identifier for non-legacy format response.push(0x00); }
142-160: Test for sign_message with legacy=true added.Good addition of a test case to validate the
sign_messagefunctionality withlegacyset totrue. Consider adding another test case forlegacy = falseto ensure both behaviors work correctly.#[test] fn test_sign_message() { let mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about".to_string(); let chain = super::SUI {}; let seed = chain.mnemonic_to_seed(mnemonic, "".to_string()).unwrap(); let path = chain.get_path(0, false); let pvk = chain.derive(seed, path).unwrap(); let message_bytes = "test message".as_bytes().to_vec(); let signature = chain.sign_message(pvk, message_bytes, true).unwrap(); assert_eq!( hex::encode(signature), "73b5a37df5ae989ec52a970547fdee96e4e76f0c668159b40a4864f2e06637cac485bfcd3e5af9a29a383243c41549c7c5b2ba645ad68aa849c6b08a53a18b02900b4d81eecea3df2f74b14200c4f4cf3f49afaca7a634ffd2cf6ff82bdaecf2" ); } + +#[test] +fn test_sign_message_non_legacy() { + let mnemonic = + "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about".to_string(); + + let chain = super::SUI {}; + let seed = chain.mnemonic_to_seed(mnemonic, "".to_string()).unwrap(); + let path = chain.get_path(0, false); + let pvk = chain.derive(seed, path).unwrap(); + + let message_bytes = "test message".as_bytes().to_vec(); + + let signature = chain.sign_message(pvk, message_bytes, false).unwrap(); + + // Update this expected value with the correct non-legacy signature + assert!(signature[0] == 0x00, "Non-legacy signature should start with 0x00"); + // Additional assertion for the complete signature if needed +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
packages/kos/src/chains/substrate/mod.rs(3 hunks)packages/kos/src/chains/sui/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kos/src/chains/substrate/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (1)
packages/kos/src/chains/sui/mod.rs (1)
71-71: Update correctly passes legacy parameter to sign_message.The call to
sign_messagehas been properly updated to passfalsefor the newlegacyparameter, maintaining consistent behavior for transaction signing.
2 participants
Summary by CodeRabbit
New Features
prepare_message_legacyandsign_message_legacy.Refactor
Tests